-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-2529 Return error if a ClientEncryption is used after Close #1785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
API Change ReportNo changes found! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this closely, we should probably use the underlying keyVaultClient
to determine if a client has been disconnected or not. mongo.ClientEncryption
appears to just wrap mongo.Client
. So instead of adding a closed
bool, we can check if a connection has ended by seeing if the pointer to the pool has been nullified. For example:
func (ce *ClientEncryption) AddKeyAltName(ctx context.Context, id bson.Binary, keyAltName string) *SingleResult {
if ce.keyVaultClient.sessionPool == nil {
return &SingleResult{err: ErrClientDisconnected}
}
}
Additionally, we only want to apply this to methods that actually use either keyValueClient
or keyVaultColl
(which is constructed from the client).
You can update the unit tests in 1 of 2 ways:
- Keep a purely mocked solution:
ce := &ClientEncryption{keyVaultClient: &Client{}}
- Actually connect to a client and then disconnect (it doesn't even matter if a server is live):
client, _ := Connect(options.Client().ApplyURI("mongodb://invalid"))
_ = client.Disconnect(context.Background())
ce := &ClientEncryption{keyVaultClient: client}
// [...]
The latter is probably better coverage.
…s, and run parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ one small suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🌟
Co-authored-by: Preston Vasquez <[email protected]>
6430128
@joyjwang The static analysis test is failing. Please run the
The race detector failure is a known issue. |
GODRIVER-2529
Summary
Return error if a ClientEncryption is used after Close
Background & Motivation